Remove `Source::for_path`

This commit removes the `Source::for_path` constructor in favor of
`Workspace::load`. This prevents re-parsing manifests multiple times as Cargo
loads up as this can sometimes be an expensive operation. Instead the
`Workspace` now retains a cache of packages that can be updated as it goes
along. Finally, this should mean that we're only parsing path dependencies at
most once rather than multiple times.

diff --git a/src/bin/commands/read_manifest.rs b/src/bin/commands/read_manifest.rs
index 5365b5bac8684bbff6f9a99dc92ecdbd8b2d5976..1e54c79e87ffd1eea076d5f6498e4d1c7da6adb8 100644 (file)
--- a/src/bin/commands/read_manifest.rs
+++ b/src/bin/commands/read_manifest.rs
@@ -1,6 +1,5 @@
 use command_prelude::*;
 
-use cargo::core::Package;
 use cargo::print_json;
 
 pub fn cli() -> App {
@@ -13,8 +12,7 @@ Print a JSON representation of a Cargo.toml manifest.",
 }
 
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
-    let root = args.root_manifest(config)?;
-    let pkg = Package::for_path(&root, config)?;
-    print_json(&pkg);
+    let ws = args.workspace(config)?;
+    print_json(&ws.current()?);
     Ok(())
 }

diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs
index d66a895540f67fea4e305bfd36830a9956664f44..f61ee52775dd446174cf7dadc376d85b5394a26e 100644 (file)
--- a/src/cargo/core/package.rs
+++ b/src/cargo/core/package.rs
@@ -81,14 +81,6 @@ impl Package {
         }
     }
 
-    /// Calculate the Package from the manifest path (and cargo configuration).
-    pub fn for_path(manifest_path: &Path, config: &Config) -> CargoResult<Package> {
-        let path = manifest_path.parent().unwrap();
-        let source_id = SourceId::for_path(path)?;
-        let (pkg, _) = ops::read_package(manifest_path, &source_id, config)?;
-        Ok(pkg)
-    }
-
     /// Get the manifest dependencies
     pub fn dependencies(&self) -> &[Dependency] {
         self.manifest.dependencies()

diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs
index 63cd778db9c77211291a9a59a5c1b5c5af3bd6a8..ee172416f27962aa21305fceee1d9375fb336a96 100644 (file)
--- a/src/cargo/core/resolver/encode.rs
+++ b/src/cargo/core/resolver/encode.rs
@@ -188,7 +188,7 @@ impl EncodableResolve {
     }
 }
 
-fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
+fn build_path_deps(ws: &Workspace) -> (HashMap<String, SourceId>) {
     // If a crate is *not* a path source, then we're probably in a situation
     // such as `cargo install` with a lock file from a remote dependency. In
     // that case we don't need to fixup any path dependencies (as they're not
@@ -207,33 +207,33 @@ fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
         visited.insert(member.package_id().source_id().clone());
     }
     for member in members.iter() {
-        build_pkg(member, ws.config(), &mut ret, &mut visited);
+        build_pkg(member, ws, &mut ret, &mut visited);
     }
     for deps in ws.root_patch().values() {
         for dep in deps {
-            build_dep(dep, ws.config(), &mut ret, &mut visited);
+            build_dep(dep, ws, &mut ret, &mut visited);
         }
     }
     for &(_, ref dep) in ws.root_replace() {
-        build_dep(dep, ws.config(), &mut ret, &mut visited);
+        build_dep(dep, ws, &mut ret, &mut visited);
     }
 
     return ret;
 
     fn build_pkg(
         pkg: &Package,
-        config: &Config,
+        ws: &Workspace,
         ret: &mut HashMap<String, SourceId>,
         visited: &mut HashSet<SourceId>,
     ) {
         for dep in pkg.dependencies() {
-            build_dep(dep, config, ret, visited);
+            build_dep(dep, ws, ret, visited);
         }
     }
 
     fn build_dep(
         dep: &Dependency,
-        config: &Config,
+        ws: &Workspace,
         ret: &mut HashMap<String, SourceId>,
         visited: &mut HashSet<SourceId>,
     ) {
@@ -245,13 +245,13 @@ fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
             Ok(p) => p.join("Cargo.toml"),
             Err(_) => return,
         };
-        let pkg = match Package::for_path(&path, config) {
+        let pkg = match ws.load(&path) {
             Ok(p) => p,
             Err(_) => return,
         };
         ret.insert(pkg.name().to_string(), pkg.package_id().source_id().clone());
         visited.insert(pkg.package_id().source_id().clone());
-        build_pkg(&pkg, config, ret, visited);
+        build_pkg(&pkg, ws, ret, visited);
     }
 }
 

diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
index d6ba3594da901ead6f29596354f2a4d31e8224cc..38be0ea29bbedcb6061acea3df53207a2afc23aa 100644 (file)
--- a/src/cargo/core/workspace.rs
+++ b/src/cargo/core/workspace.rs
@@ -1,17 +1,19 @@
-use std::collections::hash_map::{Entry, HashMap};
+use std::cell::RefCell;
 use std::collections::BTreeMap;
+use std::collections::hash_map::{Entry, HashMap};
 use std::path::{Path, PathBuf};
 use std::slice;
 
 use glob::glob;
 use url::Url;
 
-use core::{EitherManifest, Package, SourceId, VirtualManifest};
 use core::{Dependency, PackageIdSpec, Profile, Profiles};
-use util::{Config, Filesystem};
+use core::{EitherManifest, Package, SourceId, VirtualManifest};
+use ops;
 use util::errors::{CargoResult, CargoResultExt};
 use util::paths;
 use util::toml::read_manifest;
+use util::{Config, Filesystem};
 
 /// The core abstraction in Cargo for working with a workspace of crates.
 ///
@@ -67,6 +69,8 @@ pub struct Workspace<'cfg> {
     // needed by the current configuration (such as in cargo install). In some
     // cases `false` also results in the non-enforcement of dev-dependencies.
     require_optional_deps: bool,
+
+    loaded_packages: RefCell<HashMap<PathBuf, Package>>,
 }
 
 // Separate structure for tracking loaded packages (to avoid loading anything
@@ -137,6 +141,7 @@ impl<'cfg> Workspace<'cfg> {
             default_members: Vec::new(),
             is_ephemeral: false,
             require_optional_deps: true,
+            loaded_packages: RefCell::new(HashMap::new()),
         };
         ws.root_manifest = ws.find_root(manifest_path)?;
         ws.find_members()?;
@@ -172,6 +177,7 @@ impl<'cfg> Workspace<'cfg> {
             default_members: Vec::new(),
             is_ephemeral: true,
             require_optional_deps,
+            loaded_packages: RefCell::new(HashMap::new()),
         };
         {
             let key = ws.current_manifest.parent().unwrap();
@@ -669,11 +675,32 @@ impl<'cfg> Workspace<'cfg> {
 
         Ok(())
     }
+
+    pub fn load(&self, manifest_path: &Path) -> CargoResult<Package> {
+        match self.packages.maybe_get(manifest_path) {
+            Some(&MaybePackage::Package(ref p)) => return Ok(p.clone()),
+            Some(&MaybePackage::Virtual(_)) => bail!("cannot load workspace root"),
+            None => {}
+        }
+
+        let mut loaded = self.loaded_packages.borrow_mut();
+        if let Some(p) = loaded.get(manifest_path).cloned() {
+            return Ok(p);
+        }
+        let source_id = SourceId::for_path(manifest_path.parent().unwrap())?;
+        let (package, _nested_paths) = ops::read_package(manifest_path, &source_id, self.config)?;
+        loaded.insert(manifest_path.to_path_buf(), package.clone());
+        Ok(package)
+    }
 }
 
 impl<'cfg> Packages<'cfg> {
     fn get(&self, manifest_path: &Path) -> &MaybePackage {
-        &self.packages[manifest_path.parent().unwrap()]
+        self.maybe_get(manifest_path).unwrap()
+    }
+
+    fn maybe_get(&self, manifest_path: &Path) -> Option<&MaybePackage> {
+        self.packages.get(manifest_path.parent().unwrap())
     }
 
     fn load(&mut self, manifest_path: &Path) -> CargoResult<&MaybePackage> {

diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
index cc92dda7324625c2ce65517f2dbc74628cd9ee05..60e4641c1aaf0d219b50b717df889173f5dd29cc 100644 (file)
--- a/src/cargo/ops/registry.rs
+++ b/src/cargo/ops/registry.rs
@@ -447,8 +447,8 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
         Some(ref name) => name.clone(),
         None => {
             let manifest_path = find_root_manifest_for_wd(None, config.cwd())?;
-            let pkg = Package::for_path(&manifest_path, config)?;
-            pkg.name().to_string()
+            let ws = Workspace::new(&manifest_path, config)?;
+            ws.current()?.package_id().name().to_string()
         }
     };
 
@@ -508,8 +508,8 @@ pub fn yank(
         Some(name) => name,
         None => {
             let manifest_path = find_root_manifest_for_wd(None, config.cwd())?;
-            let pkg = Package::for_path(&manifest_path, config)?;
-            pkg.name().to_string()
+            let ws = Workspace::new(&manifest_path, config)?;
+            ws.current()?.package_id().name().to_string()
         }
     };
     let version = match version {